-
Notifications
You must be signed in to change notification settings - Fork 105
Add conversational search #774
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds Chats API support: new ChatWorkspaces endpoint, workspace settings trait, DTOs, client wiring (client->chats), HTTP postStream streaming support, index-level chat/embedders settings, and tests for workspace CRUD, listing, and streaming completions. (<=50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Client
participant Chats as ChatWorkspaces
participant HTTP as Http
App->>Client: client->chats->listWorkspaces()
Client->>Chats: listWorkspaces()
Chats->>HTTP: GET /chats
HTTP-->>Chats: 200 JSON results
Chats-->>Client: ChatWorkspacesResults
Client-->>App: results
sequenceDiagram
participant App
participant Client
participant Chats as ChatWorkspaces
participant HTTP as Http
participant Stream as Stream
App->>Client: client->chatWorkspace("my-assistant")->updateSettings(payload)
Client->>Chats: workspace("my-assistant")->updateSettings(payload)
Chats->>HTTP: PATCH /chats/my-assistant/settings (JSON)
HTTP-->>Chats: 200 ChatWorkspaceSettings
Chats-->>Client: ChatWorkspaceSettings
Client-->>App: settings
App->>Client: client->chatWorkspace("my-assistant")->streamCompletion(options)
Client->>Chats: workspace("my-assistant")->streamCompletion(options)
Chats->>HTTP: POST /chats/my-assistant/chat/completions (stream)
HTTP-->>Stream: chunked data
Stream-->>App: StreamInterface (read chunks)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (20)
src/Contracts/Http.php (1)
50-54
: postStream contract addition looks good; consider documenting all thrown exceptions for parityThe implementation (src/Http/Client.php::postStream) can also throw ClientExceptionInterface and CommunicationException. To keep interface docs aligned and help consumers, consider adding these to the docblock.
Apply this docblock tweak:
/** * @throws ApiException * @throws JsonEncodingException + * @throws \Psr\Http\Client\ClientExceptionInterface + * @throws \Meilisearch\Exceptions\CommunicationException */ public function postStream(string $path, $body = null, array $query = []): \Psr\Http\Message\StreamInterface;src/Client.php (1)
59-59
: Initialize $this->chats endpoint early; confirm typed property exists in the traitAssuming HandlesChatWorkspaces declares a typed public property for $chats, this avoids dynamic property issues on PHP 8.2+. If it's not declared there, we should add it.
I can open a small follow-up to add a typed property declaration if needed.
tests/Settings/ChatTest.php (1)
16-21
: Optional: use heredoc/nowdoc for multi-line documentTemplate for readabilityThe embedded newline in a single-quoted string is fine but a heredoc/nowdoc improves readability of the template.
Option:
- 'documentTemplate' => '{% for field in fields %}{% if field.is_searchable and field.value != nil %}{{ field.name }}: {{ field.value }} -{% endif %}{% endfor %}', + 'documentTemplate' => <<<'TPL' +{% for field in fields %}{% if field.is_searchable and field.value != nil %}{{ field.name }}: {{ field.value }} +{% endif %}{% endfor %} +TPL,src/Http/Client.php (1)
148-156
: Consider setting standard header casing and SSE Accept for streamingHeaders are case-insensitive, but using canonical casing is cleaner. Also, conversational streaming often returns text/event-stream; proactively sending Accept can improve content negotiation. If you prefer to keep postStream generic, we can add Accept at the caller level once the Http interface supports custom headers.
Apply:
- return $this->executeStream($request, ['Content-type' => 'application/json']); + return $this->executeStream($request, [ + 'Content-Type' => 'application/json', + 'Accept' => 'text/event-stream', + ]);src/Endpoints/Delegates/HandlesChatWorkspaces.php (2)
12-12
: Public property may be too permissive; consider tightening mutabilityExposing
$chats
as a public mutable property invites accidental reassignments. If you need it publicly accessible (to keep$client->chats
), consider making it effectively read-only (e.g., via a getter, or documenting/guarding the assignment in the owning class).
17-20
: Method naming nit: prefer “list” over “get” for collection endpointsMinor naming nit to align with endpoint semantics:
listChatWorkspaces()
would mirror the underlyinglistWorkspaces()
. Not a blocker.src/Endpoints/Delegates/HandlesSettings.php (1)
505-541
: Docblock is comprehensive; consider factoring the searchParameters shape into a reusable typeYou’ve inlined a large array shape for
searchParameters
twice (getChat/updateChat). Consider extracting a shared phpdoc typedef (via a dedicated value object/contract or a @phpstan-type alias) to avoid drift and keep it consistent with search settings elsewhere.src/Contracts/ChatWorkspacesResults.php (2)
13-20
: Guard against missing offset/limit to avoid noticesIf the API omits offset/limit (e.g., on older servers or different defaults), accessing
$params['offset']
/$params['limit']
will trigger notices. Safer defaults maintain robustness without changing behavior when fields are present.Suggested change:
- $this->offset = $params['offset']; - $this->limit = $params['limit']; + $this->offset = $params['offset'] ?? 0; + $this->limit = $params['limit'] ?? \count($this->data);If the Chats list endpoint always returns offset/limit/total, feel free to keep strict access. Otherwise, defaulting avoids runtime notices.
55-58
: Minor: count() duplicates parent::count()The parent Data already implements Countable. You can drop this override or delegate to
parent::count()
to avoid duplication.- public function count(): int - { - return \count($this->data); - } + public function count(): int + { + return parent::count(); + }tests/Endpoints/ChatWorkspaceTest.php (3)
25-31
: Enable experimental features once per test class to reduce overheadOptional: switch to setUpBeforeClass to toggle
/experimental-features
once, reducing per-test overhead. Not required, just cleaner/faster.
7-8
: Avoid naming collision with the high-level Client; alias the HTTP Client importImporting
Meilisearch\Http\Client
asClient
can be confusing alongside$this->client
(the high-level client). Alias it to improve readability.-use Meilisearch\Http\Client; +use Meilisearch\Http\Client as HttpClient;And update the instantiation:
- $http = new Client($this->host, getenv('MEILISEARCH_API_KEY')); + $http = new HttpClient($this->host, getenv('MEILISEARCH_API_KEY'));
91-131
: Streaming test is pragmatic; consider a more defensive guard to reduce flakinessThe
maxChunks
safety is good. If the backend is slow or verbose, this can still flake. Consider:
- Lowering
maxChunks
(e.g., 200) and/or- Skipping the test unless a known-good chat provider/config is available (env flag), or
- Parsing SSE “data:” lines to assert structured progress instead of raw length.
Not a blocker.
If CI occasionally flakes here, I can propose a provider-gated skip mechanism or SSE-aware parser for more deterministic assertions.
src/Contracts/ChatWorkspaceSettings.php (1)
75-87
: toArray includes nulls; ensure it’s not used for PATCH bodies as-isIf this
toArray()
ever feeds PATCH bodies, sending explicit nulls could clear server-side values. Current code paths send raw arrays directly, so this is fine; just noting for future usage.src/Endpoints/Delegates/HandlesChatWorkspaceSettings.php (7)
16-23
: URL-encode the workspace identifier when composing the pathWorkspace identifiers can contain characters that should be percent-encoded in URLs (spaces, slashes, unicode, etc.). Encode the path segment to avoid routing errors and subtle bugs.
Apply this diff:
- $response = $this->http->get('/chats/'.$this->workspaceName.'/settings'); + $workspace = rawurlencode($this->workspaceName); + $response = $this->http->get('/chats/'.$workspace.'/settings');
28-37
: Verify allowed values forsource
(string union looks potentially inconsistent with API naming)Double-check the provider identifiers against the server API. For example, docs often use lower-case
openai
or hyphenatedazure-openai
, andvllm
(all lower-case). Current values ('openAi'
,'azureOpenAi'
,'vLlm'
) may not match server expectations.If needed, consider centralizing these values in an enum or constants to avoid drift.
I can propose an enum/constant set once the canonical values are confirmed.
41-48
: Also encode the workspace in update pathSame reasoning as for GET: encode the workspace path segment.
- $response = $this->http->patch('/chats/'.$this->workspaceName.'/settings', $settings); + $workspace = rawurlencode($this->workspaceName); + $response = $this->http->patch('/chats/'.$workspace.'/settings', $settings);
55-62
: Confirm DELETE semantics vs method name, and encode workspace
- The PR summary mentions “DELETE /chats/{workspace_uid}/settings — delete a workspace.” The method here is named
resetSettings()
and the docblock says “Reset … to default values” while returningChatWorkspaceSettings
. Please verify the server semantics:
- If it actually deletes the workspace (likely 204/empty body), consider renaming to
deleteWorkspace()
and returnvoid
/array
.- If it resets settings and returns the resulting settings, then the name and return type are fine.
- Also encode the workspace segment as with GET/PATCH.
- $response = $this->http->delete('/chats/'.$this->workspaceName.'/settings'); + $workspace = rawurlencode($this->workspaceName); + $response = $this->http->delete('/chats/'.$workspace.'/settings');If the endpoint deletes the workspace and returns no content, adjust the signature and return accordingly:
- public function resetSettings(): ChatWorkspaceSettings + public function deleteWorkspace(): void { if (null === $this->workspaceName) { - throw new \InvalidArgumentException('Workspace name is required to reset settings'); + throw new \InvalidArgumentException('Workspace name is required to delete a workspace'); } - - $response = $this->http->delete('/chats/'.$this->workspaceName.'/settings'); - - return new ChatWorkspaceSettings($response); + $workspace = rawurlencode($this->workspaceName); + $this->http->delete('/chats/'.$workspace.'/settings'); }
67-72
: Tighten/clarify the options docblock; the method can enforcestream
- You can restrict
role
to known values ('system'|'user'|'assistant'
), which improves IDE/static analysis.- Since the method is “streamCompletion”, you can treat
stream
as optional and document that the method forcesstream=true
.- * @param array{ - * model: string, - * messages: array<array{role: string, content: string}>, - * stream: bool - * } $options The request body for the chat completion + * @param array{ + * model: string, + * messages: array<array{role: 'system'|'user'|'assistant', content: string}>, + * stream?: bool + * } $options The request body for the chat completion. Note: this method forces stream=true.
75-80
: Force streaming flag and encode workspace in completion pathEnsure
stream
is set to true (it’s easy to forget from the caller) and encode the workspace path segment.public function streamCompletion(array $options): \Psr\Http\Message\StreamInterface { if (null === $this->workspaceName) { throw new \InvalidArgumentException('Workspace name is required for chat completion'); } - return $this->http->postStream('/chats/'.$this->workspaceName.'/chat/completions', $options); + $options['stream'] = true; + $workspace = rawurlencode($this->workspaceName); + return $this->http->postStream('/chats/'.$workspace.'/chat/completions', $options); }
7-10
: Add trait-level phpdoc for host expectations ($http, $workspaceName)Helps Psalm/PHPStan understand that the trait expects these members on the host class, improving static analysis without changing runtime behavior.
namespace Meilisearch\Endpoints\Delegates; use Meilisearch\Contracts\ChatWorkspaceSettings; +/** + * @property \Meilisearch\Contracts\Http $http + * @property string|null $workspaceName + */ trait HandlesChatWorkspaceSettings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
src/Client.php
(3 hunks)src/Contracts/ChatWorkspaceSettings.php
(1 hunks)src/Contracts/ChatWorkspacesResults.php
(1 hunks)src/Contracts/Http.php
(1 hunks)src/Endpoints/ChatWorkspaces.php
(1 hunks)src/Endpoints/Delegates/HandlesChatWorkspaceSettings.php
(1 hunks)src/Endpoints/Delegates/HandlesChatWorkspaces.php
(1 hunks)src/Endpoints/Delegates/HandlesSettings.php
(2 hunks)src/Http/Client.php
(2 hunks)tests/Endpoints/ChatWorkspaceTest.php
(1 hunks)tests/Settings/ChatTest.php
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (11)
src/Endpoints/ChatWorkspaces.php (6)
src/Meilisearch.php (1)
Meilisearch
(7-18)src/Contracts/ChatWorkspacesResults.php (2)
ChatWorkspacesResults
(7-59)__construct
(13-20)src/Contracts/Endpoint.php (1)
Endpoint
(7-23)src/Contracts/ChatWorkspaceSettings.php (1)
__construct
(18-30)src/Client.php (1)
__construct
(50-71)src/Http/Client.php (2)
__construct
(44-63)get
(70-78)
src/Contracts/ChatWorkspacesResults.php (2)
src/Contracts/Data.php (1)
Data
(7-50)src/Contracts/ChatWorkspaceSettings.php (2)
__construct
(18-30)toArray
(75-87)
src/Contracts/ChatWorkspaceSettings.php (2)
src/Contracts/Data.php (1)
Data
(7-50)src/Contracts/ChatWorkspacesResults.php (2)
__construct
(13-20)toArray
(45-53)
src/Endpoints/Delegates/HandlesChatWorkspaceSettings.php (3)
src/Contracts/ChatWorkspaceSettings.php (1)
ChatWorkspaceSettings
(7-88)src/Contracts/Http.php (4)
get
(17-17)patch
(42-42)delete
(48-48)postStream
(54-54)src/Http/Client.php (4)
get
(70-78)patch
(122-130)delete
(132-140)postStream
(148-156)
src/Endpoints/Delegates/HandlesChatWorkspaces.php (2)
src/Contracts/ChatWorkspacesResults.php (1)
ChatWorkspacesResults
(7-59)src/Endpoints/ChatWorkspaces.php (3)
ChatWorkspaces
(12-37)listWorkspaces
(26-31)workspace
(33-36)
src/Contracts/Http.php (1)
src/Http/Client.php (1)
postStream
(148-156)
tests/Endpoints/ChatWorkspaceTest.php (5)
src/Meilisearch.php (1)
Meilisearch
(7-18)src/Http/Client.php (2)
Client
(25-261)patch
(122-130)src/Endpoints/ChatWorkspaces.php (2)
workspace
(33-36)listWorkspaces
(26-31)src/Endpoints/Delegates/HandlesChatWorkspaceSettings.php (4)
updateSettings
(39-48)getSettings
(14-23)resetSettings
(53-62)streamCompletion
(73-80)src/Contracts/ChatWorkspaceSettings.php (8)
getSource
(32-35)getOrgId
(37-40)getProjectId
(42-45)getApiVersion
(47-50)getDeploymentId
(52-55)getBaseUrl
(57-60)getPrompts
(70-73)getApiKey
(62-65)
src/Client.php (1)
src/Endpoints/ChatWorkspaces.php (1)
ChatWorkspaces
(12-37)
src/Http/Client.php (5)
src/Contracts/Http.php (1)
postStream
(54-54)src/Http/Serialize/SerializerInterface.php (2)
serialize
(21-21)unserialize
(30-30)src/Http/Serialize/Json.php (2)
serialize
(15-24)unserialize
(26-35)src/Exceptions/ApiException.php (1)
ApiException
(9-95)src/Exceptions/CommunicationException.php (1)
CommunicationException
(7-13)
tests/Settings/ChatTest.php (4)
src/Endpoints/Indexes.php (1)
Indexes
(24-278)src/Endpoints/Delegates/HandlesIndex.php (1)
index
(25-28)src/Endpoints/Delegates/HandlesSettings.php (2)
getChat
(542-545)updateChat
(584-587)src/Endpoints/Delegates/HandlesTasks.php (1)
waitForTask
(45-48)
src/Endpoints/Delegates/HandlesSettings.php (4)
src/Contracts/Http.php (2)
get
(17-17)patch
(42-42)src/Http/Client.php (2)
get
(70-78)patch
(122-130)src/Endpoints/Keys.php (1)
get
(153-158)src/Endpoints/Tasks.php (1)
get
(16-19)
🔇 Additional comments (12)
src/Client.php (1)
36-36
: Good addition: exposes chat workspaces via HandlesChatWorkspacesWiring the chat workspaces endpoint through the trait is consistent with the existing client design.
tests/Settings/ChatTest.php (1)
53-57
: Confirm HTTP verb for updating index chat settings (PATCH vs PUT) — PATCH is correctMeilisearch docs and the Settings API spec use PATCH for /indexes/{index_uid}/settings/chat; meilisearch.dev appears to have a typo that says PUT while its example uses PATCH. The current use of PATCH is canonical and tests are correct.
- tests/Settings/ChatTest.php (lines 53–57) — no change required.
- HandlesSettings::updateChat — uses PATCH — OK.
src/Endpoints/ChatWorkspaces.php (3)
20-24
: Constructor and workspace scoping look goodStoring the optional workspace name here aligns with the trait-based operations that need it.
26-31
: listWorkspaces: OK; assumes standard list payloadReturning ChatWorkspacesResults directly from the GET response is consistent with other endpoints. Ensure the API always includes results/offset/limit/total as expected by the DTO.
33-36
: Fluent workspace() helper: LGTMThis mirrors the fluent style used elsewhere in the client.
src/Endpoints/Delegates/HandlesChatWorkspaces.php (1)
10-28
: Solid trait and API surface; exposes chat workspaces cleanlyThe trait looks consistent with the rest of the client’s delegation pattern and provides a clear entry point via
$client->chats
. The helper methods are thin and correct.src/Endpoints/Delegates/HandlesSettings.php (2)
421-445
: Embedders settings endpoints look correct and consistentThe GET/PATCH/DELETE trio and @SInCE annotations align with the existing settings conventions. No issues spotted here.
542-587
: No change — PATCH is the correct verb for updating index chat settingsMeilisearch docs specify PATCH for /indexes/{index_uid}/settings/chat (Update index chat settings). The code already uses PATCH, so the suggested change to PUT is incorrect — do not change the verb.
- Location to note:
- src/Endpoints/Delegates/HandlesSettings.php
- getChat(): returns GET for .../settings/chat (line ~544)
- updateChat(): returns PATCH for .../settings/chat (line ~586) — correct
Docs: https://www.meilisearch.com/docs/reference/api/settings — see "Update index chat settings"
Likely an incorrect or invalid review comment.
src/Contracts/ChatWorkspacesResults.php (1)
22-28
: Return type annotation for results is precise and helpfulThe
@return array<int, array{uid: string}>
keeps consumers aware of the shape. Nicely done.tests/Endpoints/ChatWorkspaceTest.php (1)
63-70
: ListWorkspaces assertion is focused and correctAsserting the presence of the workspace by UID keeps the test minimal and robust. Looks good.
src/Contracts/ChatWorkspaceSettings.php (1)
18-30
: DTO is clear and aligns with the API response shapeConstructor defaults and getters match what the endpoints return (including masked apiKey). The
prompts
shape annotation is helpful for consumers.src/Endpoints/Delegates/HandlesChatWorkspaceSettings.php (1)
9-11
: LGTM: Well-scoped trait with clear, typed APIClean separation of workspace chat settings concerns, strict types, and typed returns are on point. The use of a PSR-7 stream for completions is appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Http/Client.php (1)
178-215
: Resolved: correct exception is now caught during streaming error parsingCatching JsonDecodingException here addresses the previous bug where \JsonException was ineffective with the current serializer. This ensures we fall back to the raw body and throw ApiException as intended.
🧹 Nitpick comments (2)
src/Http/Client.php (2)
142-156
: Add explicit SSE Accept header (and minor header casing nit) for streaming requestsGreat addition. To improve interoperability with proxies and servers that require content negotiation for streaming, set an explicit Accept header for SSE. Also, prefer canonical header casing for readability.
Apply:
- return $this->executeStream($request, ['Content-type' => 'application/json']); + return $this->executeStream($request, [ + 'Content-Type' => 'application/json', + 'Accept' => 'text/event-stream', + ]);
178-215
: Broaden JSON Content-Type detection to include +json variants (e.g., application/problem+json)isJSONResponse currently matches only application/json. Many APIs return JSON errors as application/problem+json or other application/*+json types. Broadening detection improves error parsing on both streaming and non-streaming paths without affecting NDJSON.
Outside the selected range, update isJSONResponse:
- /** - * Checks if any of the header values indicate a JSON response. - * - * @param array $headerValues the array of header values to check - * - * @return bool true if any header value contains 'application/json', otherwise false - */ + /** + * Checks if any of the header values indicate a JSON response. + * Matches 'application/json' and any 'application/*+json' (e.g., application/problem+json). + * + * @param array $headerValues the array of header values to check + * + * @return bool true if any header value is JSON-based, otherwise false + */ private function isJSONResponse(array $headerValues): bool { - $filteredHeaders = array_filter($headerValues, static function (string $headerValue) { - return false !== strpos($headerValue, 'application/json'); - }); + $filteredHeaders = array_filter($headerValues, static function (string $headerValue) { + $value = strtolower($headerValue); + return false !== strpos($value, 'application/json') + || false !== strpos($value, '+json'); + }); return \count($filteredHeaders) > 0; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/Http/Client.php
(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Http/Client.php (6)
src/Contracts/Http.php (1)
postStream
(54-54)src/Http/Serialize/SerializerInterface.php (2)
serialize
(21-21)unserialize
(30-30)src/Http/Serialize/Json.php (2)
serialize
(15-24)unserialize
(26-35)src/Exceptions/JsonDecodingException.php (1)
JsonDecodingException
(7-13)src/Exceptions/ApiException.php (1)
ApiException
(9-95)src/Exceptions/CommunicationException.php (1)
CommunicationException
(7-13)
Pull Request
Related issue
Fixes #765
What does this PR do?
Add methods to interact with chat workspaces endpoints:
PATCH /chats/{workspace}/settings
)GET /chats/{workspace}/settings
)GET /chats
)DELETE /chats/{workspace_uid}/settings
POST chats/{workspace}/chat/completions
)Add methods to interact with index chat settings:
GET /indexes/{index_uid}/settings/chat
PUT /indexes/{index_uid}/settings/chat
PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit
New Features
Settings
Tests